feat: add local analysis orchestration#46
Conversation
|
@coderabbitai review |
📝 WalkthroughSummary by CodeRabbit
Walkthrough로컬 분석 작업 오케스트레이션이 추가되었습니다. React 프론트엔드의 폴링 기반 UI, Tauri/Rust 백엔드의 typed IPC 명령( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
rect rgba(200,220,255,0.5)
actor User
participant React as "React Frontend"
participant Tauri as "Tauri / Rust"
participant Python as "Python Engine"
participant Store as "In-Memory JobStore"
end
User->>React: Start Analysis 클릭 (요청)
React->>Tauri: invoke start_analysis_job(request)
activate Tauri
Tauri->>Store: jobId 생성 및 상태 저장(queued)
Tauri->>Python: 시작 서브프로세스 (stdin JSON)
Tauri-->>React: 초기 AnalysisJobStatus(queued)
deactivate Tauri
loop 폴링 (ANALYSIS_POLL_INTERVAL_MS)
React->>Tauri: invoke get_analysis_job_status(jobId)
activate Tauri
Tauri->>Store: 상태 조회
Tauri-->>React: AnalysisJobStatus(queued/running/succeeded/failed)
deactivate Tauri
React->>React: UI 업데이트
end
rect rgba(200,255,200,0.5)
Python->>Python: 요청 검증 및 분석 실행
Python-->>Tauri: 결과 JSON (stdout)
Tauri->>Store: 상태 업데이트 (succeeded/failed + result/error)
end
React->>User: 최종 결과 렌더링
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50분 Possibly related PRs
정리이 PR은 로컬 분석 작업 오케스트레이션 시스템을 추가합니다. Tauri IPC 명령(start_analysis_job, get_analysis_job_status)을 구현하여 데스크톱 Rust 백엔드와 Python 분석 엔진을 연결하고, React 프론트엔드에서 폴링 기반 작업 상태 추적 UI를 제공합니다.
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 15
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src-tauri/src/main.rs`:
- Around line 22-26: The jobs HashMap in AppStateInner (jobs) accumulates
completed AnalysisJobStatus/RehearsalSongPayload entries with no deletion,
causing unbounded memory growth; modify the code paths that insert/overwrite
jobs (where AnalysisJobStatus and RehearsalSongPayload are stored) to evict
entries once they reach a terminal state or implement an eviction policy: either
add a timestamp/expiry field and run a periodic cleanup task to remove entries
older than TTL, or switch to an LRU bounded cache (e.g., lru::LruCache) to cap
size, and ensure read/update functions (the functions that return job results)
remove the job after first successful read if desired; also ensure thread safety
when replacing the Mutex<HashMap<...>> with the chosen eviction mechanism.
- Around line 380-392: In the failure branch where stdin.write_all(...) returns
Err, after calling process.kill() you must also reap the child to avoid leaving
a zombie; call process.wait() (or process.wait_timeout/try_wait if nonblocking
behavior is required) and ignore or log any error before returning
failed_status(...) so the Child started earlier is properly waited on; update
the branch referencing stdin.write_all, process.kill, and failed_status (and
keep requested_at and AnalysisJobErrorCode usage) to ensure the child is
waited/reaped after kill.
- Around line 359-361: 현재 Command 빌더에서 .stdout(Stdio::piped()) 및
.stderr(Stdio::piped())로 파이프를 열어놓고, 이후 폴링 루프에서 process.try_wait()만 호출(폴링 루프 내부의
"process.try_wait()" 사용)하므로 자식 프로세스가 stdout/stderr를 버퍼 한도까지 쓸 경우 쓰기 블록으로 이어져
데드락이 발생합니다; 해결하려면 파이프를 읽는 로직을 추가하세요 — 자식 프로세스에서 반환되는 ChildStdout/ChildStderr를 별도
스레드로 spawn하여(또는 비동기 태스크로) 동시에 drain하도록 하거나, stderr를 사용하지 않으면
.stderr(Stdio::null())로 변경하여 파이프를 제거하시기 바랍니다; 해당 변경은 Command 생성 코드(현재
.stdin/.stdout/.stderr 체인)와 폴링 루프에서 process.try_wait()를 호출하는 부분을 함께 수정해 적용하세요.
In `@apps/desktop/src/App.tsx`:
- Around line 155-156: jobStatus and jobError are currently rendered as plain
<p> elements so screen readers miss updates; change the JSX that renders
jobStatus (where progressMessage(t, jobStatus.state) is used) to an element with
role="status" and aria-live="polite" (e.g., a <div> or <p> wrapping the
progressMessage) and change the JSX that renders jobError to an element with
role="alert" so failures are announced immediately; ensure you still use the
existing jobStatus, jobError and progressMessage(t, ...) symbols and only add
the appropriate ARIA attributes/roles to those render nodes.
In `@apps/desktop/src/i18n/index.ts`:
- Around line 5-15: TranslationKey is currently derived from enCommon only, so
missing keys in other locale objects (koCommon) are only caught at runtime;
change the dictionaries typing to force compile-time synchronization by typing
dictionaries as Record<Locale, Record<TranslationKey, string>> (or deriving a
Dictionaries type that maps Locale to Record<keyof typeof enCommon, string>) and
adjust createTranslator to accept Locale and index dictionaries with that type;
ensure enCommon and koCommon conform to that shared Record type so any missing
translation keys in koCommon will cause a TypeScript error during compilation.
In `@apps/desktop/src/lib/analysis.ts`:
- Around line 22-28: The getInvoke function currently returns the imported
invoke even in browsers, causing runtime errors; change it to first import and
call isTauri() from "@tauri-apps/api/core" and only return invoke when isTauri()
is true and window.__TAURI_INVOKE__ is available; otherwise return null so
browserFallback() can run. Update the getInvoke implementation (referencing
getInvoke, window.__TAURI_INVOKE__, invoke, and isTauri) to perform the runtime
check and adjust the return values accordingly.
In `@docs/plans/2026-03-12-issue-32-analysis-orchestration-design.md`:
- Around line 66-73: 문서상 데이터 흐름이 queued에서 바로 terminal(succeeded/failed)로만 기록되도록
되어 있어 UI가 running 상태를 알 수 없으니, start_analysis_job 흐름에 subprocess를 spawn한 직후(또는
subprocess가 실제로 시작된 시점)에 상태를 `running`으로 전이하도록 명시하고 문서의 단계 3과 5 사이에 `running`
저장을 추가하세요; 구체적으로 문서에서 언급된 `start_analysis_job`, `AnalysisJobRequest`, 상태
값(`queued`, `running`, `succeeded`, `failed`) 및 폴링 엔드포인트
`get_analysis_job_status`를 업데이트하여 Rust가 subprocess 시작 시점에 `running`을 기록하고 폴링 결과가
이를 반영하도록 설명을 보강하세요.
- Around line 82-96: The test plan omits failure scenarios for hung
subprocesses, broken stdout/invalid JSON, and concurrent execution limits;
update the docs and tests to add scenarios that simulate: (1) start_analysis_job
returning engine_unavailable when the Python subprocess times out or is killed,
(2) malformed/invalid JSON on subprocess stdout and mapping to a typed safe
failure (e.g., JSON parse error path), (3) repeated start_analysis_job calls
exercising and enforcing the in-flight job limit and validating cleanup and
resource reclamation, and (4) process kill/cleanup behavior by asserting
subprocess termination, orphan prevention, and that errors are redacted in logs;
reference the existing IPC boundary and subprocess invocation code paths so
tests exercise the allowlist/validation, subprocess argument arrays, and error
mapping logic.
In `@docs/plans/2026-03-12-issue-32-analysis-orchestration.md`:
- Line 11: Change the task headings so they follow a proper H2 level under the
document title: replace the H3 heading "Task 1: Add shared analysis job
contracts" (and the equivalent "Task ..." headings later in the file) with H2
(##) headings, or alternatively introduce a new H2 parent section under the main
title and demote those task headings to H3 (###); update the headings for all
task sections referenced (the repeated "Task ..." headings) so markdownlint
MD001 is resolved and the heading hierarchy is consistent.
In `@packages/shared-types/src/index.ts`:
- Around line 105-113: The AnalysisJobSnapshot type currently duplicates failure
information by having both status (which contains error info) and a separate
error?: AnalysisJobError field; remove this ambiguity by deleting the top-level
error property from AnalysisJobSnapshot and rely on status.error as the single
source of truth, or if a distinct error meaning is required, rename error to a
clearly differentiated identifier (e.g., externalError or legacyError) and add a
doc comment explaining how it differs from status.error; update all
references/usages of AnalysisJobSnapshot, AnalysisJobSnapshot.error, and any
code expecting AnalysisJobError to use the chosen single source or new name so
consumers remain consistent.
- Around line 95-103: The current AnalysisJobStatus type allows invalid
state/result combinations; change it from a single object type to a
discriminated union keyed by the state field so the compiler enforces which
fields are present for each state (e.g., pending/processing states allow
optional progressLabel but neither result nor error, a success/finished state
requires result: RehearsalSong, and a failed state requires error:
AnalysisJobError); keep common fields (jobId, requestedAt, updatedAt) in each
variant and update any consumers to handle the narrowed union (refer to
AnalysisJobStatus, AnalysisJobState, RehearsalSong, and AnalysisJobError to
locate and adjust the type).
In `@services/analysis-engine/src/bandscope_analysis/api.py`:
- Around line 280-286: The success response currently mixes a dynamic
progressLabel with a hardcoded demo song title from build_demo_rehearsal_song(),
causing test mismatches; update run_analysis_job to ensure the returned
result.title matches request['sourceLabel']—either by changing
build_demo_rehearsal_song to accept a title parameter and pass
request['sourceLabel'], or by overriding result['title'] =
request['sourceLabel'] after calling build_demo_rehearsal_song(); keep all other
fields (jobId, state, timestamps, progressLabel) unchanged so the contract stays
consistent.
- Around line 141-145: The validation for role_focus currently only checks type
and element types but allows an empty list; update the validation in the same
block that uses role_focus to reject empty lists by adding a check like "if not
role_focus: raise ValueError(...)" so that an empty array raises a clear
ValueError for the 'roleFocus' field (keep the existing element-type checks
using enumerate for role strings). Ensure you reference the role_focus variable
and the existing ValueError pattern for 'roleFocus' so the new check is
colocated with the current isinstance and per-element string checks.
In `@services/analysis-engine/tests/test_api.py`:
- Around line 64-66: The test is only asserting
manualOverrides[0].value["source"], missing validation of the full override
payload which lets typoed keys (e.g., value.cord) pass; update the test in
services/analysis-engine/tests/test_api.py to assert the entire
manualOverrides[0]["value"] matches the expected HarmonyPayload shape (including
the "chord" key and its expected content) or at minimum assert presence and
correctness of required keys like "chord" and "source" on
song["sections"][0]["roles"][2]["manualOverrides"][0]["value"]; reference the
song variable and the manualOverrides/HarmonyPayload contract so the test fails
if the value is malformed or has typos.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8f71187a-3310-47e5-bdac-70811c02b878
⛔ Files ignored due to path filters (6)
apps/desktop/src-tauri/Cargo.lockis excluded by!**/*.lockapps/desktop/src-tauri/gen/schemas/acl-manifests.jsonis excluded by!**/gen/**apps/desktop/src-tauri/gen/schemas/capabilities.jsonis excluded by!**/gen/**apps/desktop/src-tauri/gen/schemas/desktop-schema.jsonis excluded by!**/gen/**apps/desktop/src-tauri/gen/schemas/macOS-schema.jsonis excluded by!**/gen/**package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (25)
ARCHITECTURE.mdapps/desktop/package.jsonapps/desktop/src-tauri/Cargo.tomlapps/desktop/src-tauri/build.rsapps/desktop/src-tauri/capabilities/main.jsonapps/desktop/src-tauri/permissions/autogenerated/get_analysis_job_status.tomlapps/desktop/src-tauri/permissions/autogenerated/start_analysis_job.tomlapps/desktop/src-tauri/src/main.rsapps/desktop/src-tauri/tauri.conf.jsonapps/desktop/src/App.test.tsxapps/desktop/src/App.tsxapps/desktop/src/i18n/index.tsapps/desktop/src/lib/analysis.tsapps/desktop/src/locales/en/common.jsonapps/desktop/src/locales/ko/common.jsondocs/architecture/overview.mddocs/plans/2026-03-12-issue-32-analysis-orchestration-design.mddocs/plans/2026-03-12-issue-32-analysis-orchestration.mddocs/security/app-security.mdpackages/shared-types/src/index.tspackages/shared-types/test/index.test.tsservices/analysis-engine/src/bandscope_analysis/api.pyservices/analysis-engine/src/bandscope_analysis/cli.pyservices/analysis-engine/tests/test_api.pyservices/analysis-engine/tests/test_cli.py
| struct AppStateInner { | ||
| next_job: AtomicU64, | ||
| in_flight_jobs: AtomicUsize, | ||
| jobs: Mutex<HashMap<String, AnalysisJobStatus>>, | ||
| } |
There was a problem hiding this comment.
완료된 작업을 영구 보관해서 세션이 길어질수록 메모리가 커집니다.
jobs는 insert/overwrite만 있고 삭제 경로가 없습니다. in-flight는 2개로 제한돼도 완료된 AnalysisJobStatus와 RehearsalSongPayload는 앱 종료 전까지 계속 남으므로, 순차 분석을 오래 돌리면 메모리가 단조 증가합니다. TTL/LRU를 두거나 terminal 상태를 한 번 읽은 뒤 제거하는 정리가 필요합니다.
Also applies to: 344-347, 529-544
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src-tauri/src/main.rs` around lines 22 - 26, The jobs HashMap in
AppStateInner (jobs) accumulates completed
AnalysisJobStatus/RehearsalSongPayload entries with no deletion, causing
unbounded memory growth; modify the code paths that insert/overwrite jobs (where
AnalysisJobStatus and RehearsalSongPayload are stored) to evict entries once
they reach a terminal state or implement an eviction policy: either add a
timestamp/expiry field and run a periodic cleanup task to remove entries older
than TTL, or switch to an LRU bounded cache (e.g., lru::LruCache) to cap size,
and ensure read/update functions (the functions that return job results) remove
the job after first successful read if desired; also ensure thread safety when
replacing the Mutex<HashMap<...>> with the chosen eviction mechanism.
| .stdin(Stdio::piped()) | ||
| .stdout(Stdio::piped()) | ||
| .stderr(Stdio::piped()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l apps/desktop/src-tauri/src/main.rsRepository: seonghobae/bandscope
Length of output: 102
🏁 Script executed:
sed -n '350,370p' apps/desktop/src-tauri/src/main.rsRepository: seonghobae/bandscope
Length of output: 720
🏁 Script executed:
sed -n '390,450p' apps/desktop/src-tauri/src/main.rsRepository: seonghobae/bandscope
Length of output: 2041
🏁 Script executed:
sed -n '350,555p' apps/desktop/src-tauri/src/main.rs | grep -A 5 -B 5 "stdout\|stderr\|output"Repository: seonghobae/bandscope
Length of output: 1361
파이프 스트림의 버퍼 오버플로우로 인한 교착 상태(Deadlock) 위험
Lines 359-361에서 stdout과 stderr를 모두 파이프로 설정한 후, Lines 390-444의 폴링 루프에서는 process.try_wait()만 호출하고 스트림을 읽지 않습니다. 자식 프로세스가 결과 JSON이나 로그를 파이프 버퍼(일반적으로 64KB) 한도까지 작성하면 write() 호출에서 블로킹되어 종료할 수 없게 됩니다. 이 상태에서 부모 프로세스는 폴링 루프에 갇혀 타임아웃을 기다리게 되며, 실제로는 정상 작동하는 프로세스도 "Analysis engine timed out."으로 잘못 진단됩니다.
해결 방법:
- 스트림을 별도 스레드에서 동시에 소비하거나
- 사용하지 않는
stderr는Stdio::null()로 설정
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src-tauri/src/main.rs` around lines 359 - 361, 현재 Command 빌더에서
.stdout(Stdio::piped()) 및 .stderr(Stdio::piped())로 파이프를 열어놓고, 이후 폴링 루프에서
process.try_wait()만 호출(폴링 루프 내부의 "process.try_wait()" 사용)하므로 자식 프로세스가
stdout/stderr를 버퍼 한도까지 쓸 경우 쓰기 블록으로 이어져 데드락이 발생합니다; 해결하려면 파이프를 읽는 로직을 추가하세요 — 자식
프로세스에서 반환되는 ChildStdout/ChildStderr를 별도 스레드로 spawn하여(또는 비동기 태스크로) 동시에 drain하도록
하거나, stderr를 사용하지 않으면 .stderr(Stdio::null())로 변경하여 파이프를 제거하시기 바랍니다; 해당 변경은
Command 생성 코드(현재 .stdin/.stdout/.stderr 체인)와 폴링 루프에서 process.try_wait()를 호출하는
부분을 함께 수정해 적용하세요.
| if let Some(mut stdin) = process.stdin.take() { | ||
| if stdin.write_all(payload.to_string().as_bytes()).is_err() { | ||
| let _ = process.kill(); | ||
| return failed_status( | ||
| payload["jobId"] | ||
| .as_str() | ||
| .unwrap_or("unknown-job") | ||
| .to_string(), | ||
| requested_at, | ||
| AnalysisJobErrorCode::EngineUnavailable, | ||
| "Analysis engine is unavailable.", | ||
| ); | ||
| } |
There was a problem hiding this comment.
stdin.write_all 실패 분기에서 child를 reap하지 않아 좀비가 남습니다.
이 경로만 kill() 후 바로 반환하고 wait()를 호출하지 않습니다. 자식이 일찍 종료해서 broken pipe가 난 경우가 흔하므로, 엔진 시작 실패가 반복되면 Unix에서 unreaped child가 누적될 수 있습니다.
🛠️ 제안하는 수정
if let Some(mut stdin) = process.stdin.take() {
if stdin.write_all(payload.to_string().as_bytes()).is_err() {
let _ = process.kill();
+ let _ = process.wait();
return failed_status(
payload["jobId"]
.as_str()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if let Some(mut stdin) = process.stdin.take() { | |
| if stdin.write_all(payload.to_string().as_bytes()).is_err() { | |
| let _ = process.kill(); | |
| return failed_status( | |
| payload["jobId"] | |
| .as_str() | |
| .unwrap_or("unknown-job") | |
| .to_string(), | |
| requested_at, | |
| AnalysisJobErrorCode::EngineUnavailable, | |
| "Analysis engine is unavailable.", | |
| ); | |
| } | |
| if let Some(mut stdin) = process.stdin.take() { | |
| if stdin.write_all(payload.to_string().as_bytes()).is_err() { | |
| let _ = process.kill(); | |
| let _ = process.wait(); | |
| return failed_status( | |
| payload["jobId"] | |
| .as_str() | |
| .unwrap_or("unknown-job") | |
| .to_string(), | |
| requested_at, | |
| AnalysisJobErrorCode::EngineUnavailable, | |
| "Analysis engine is unavailable.", | |
| ); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src-tauri/src/main.rs` around lines 380 - 392, In the failure
branch where stdin.write_all(...) returns Err, after calling process.kill() you
must also reap the child to avoid leaving a zombie; call process.wait() (or
process.wait_timeout/try_wait if nonblocking behavior is required) and ignore or
log any error before returning failed_status(...) so the Child started earlier
is properly waited on; update the branch referencing stdin.write_all,
process.kill, and failed_status (and keep requested_at and AnalysisJobErrorCode
usage) to ensure the child is waited/reaped after kill.
| {jobStatus ? <p>{progressMessage(t, jobStatus.state)}</p> : null} | ||
| {jobError ? <p>{jobError}</p> : null} |
There was a problem hiding this comment.
상태/오류 업데이트를 plain <p>로만 렌더링하면 스크린리더가 변경을 놓칩니다.
분석 시작 이후의 핵심 피드백이 polling으로 갱신되는데, Line 155-156은 live region이 아니라서 queued/running/failed/succeeded 전환이 자동으로 announce되지 않습니다. 진행 상태는 role="status"와 aria-live="polite", 실패는 role="alert"로 노출해 주세요.
♿ 제안하는 수정
- {jobStatus ? <p>{progressMessage(t, jobStatus.state)}</p> : null}
- {jobError ? <p>{jobError}</p> : null}
+ {jobStatus ? (
+ <p role="status" aria-live="polite">
+ {progressMessage(t, jobStatus.state)}
+ </p>
+ ) : null}
+ {jobError ? <p role="alert">{jobError}</p> : null}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {jobStatus ? <p>{progressMessage(t, jobStatus.state)}</p> : null} | |
| {jobError ? <p>{jobError}</p> : null} | |
| {jobStatus ? ( | |
| <p role="status" aria-live="polite"> | |
| {progressMessage(t, jobStatus.state)} | |
| </p> | |
| ) : null} | |
| {jobError ? <p role="alert">{jobError}</p> : null} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/App.tsx` around lines 155 - 156, jobStatus and jobError are
currently rendered as plain <p> elements so screen readers miss updates; change
the JSX that renders jobStatus (where progressMessage(t, jobStatus.state) is
used) to an element with role="status" and aria-live="polite" (e.g., a <div> or
<p> wrapping the progressMessage) and change the JSX that renders jobError to an
element with role="alert" so failures are announced immediately; ensure you
still use the existing jobStatus, jobError and progressMessage(t, ...) symbols
and only add the appropriate ARIA attributes/roles to those render nodes.
| export type TranslationKey = keyof typeof enCommon; | ||
|
|
||
| const dictionaries = { | ||
| en: enCommon, | ||
| ko: koCommon | ||
| } as const; | ||
|
|
||
| export function createTranslator(locale: Locale = "en") { | ||
| return function t(key: keyof typeof enCommon): string { | ||
| return function t(key: TranslationKey): string { | ||
| return dictionaries[locale][key] ?? dictionaries.en[key]; | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
로케일 키 동기화를 타입으로 강제해 두는 편이 안전합니다.
지금 구조는 TranslationKey를 영어 사전에서만 뽑고, 다른 로케일 누락은 런타임 fallback으로만 발견됩니다. dictionaries 자체를 Record<Locale, Record<TranslationKey, string>>로 고정해 두면 새 키 추가 시 로케일 불일치를 컴파일 단계에서 바로 잡을 수 있습니다.
♻️ 제안 diff
export type Locale = "en" | "ko";
export type TranslationKey = keyof typeof enCommon;
+type TranslationDictionary = Record<TranslationKey, string>;
-const dictionaries = {
+const dictionaries: Record<Locale, TranslationDictionary> = {
en: enCommon,
ko: koCommon
-} as const;
+};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type TranslationKey = keyof typeof enCommon; | |
| const dictionaries = { | |
| en: enCommon, | |
| ko: koCommon | |
| } as const; | |
| export function createTranslator(locale: Locale = "en") { | |
| return function t(key: keyof typeof enCommon): string { | |
| return function t(key: TranslationKey): string { | |
| return dictionaries[locale][key] ?? dictionaries.en[key]; | |
| }; | |
| export type TranslationKey = keyof typeof enCommon; | |
| type TranslationDictionary = Record<TranslationKey, string>; | |
| const dictionaries: Record<Locale, TranslationDictionary> = { | |
| en: enCommon, | |
| ko: koCommon | |
| }; | |
| export function createTranslator(locale: Locale = "en") { | |
| return function t(key: TranslationKey): string { | |
| return dictionaries[locale][key] ?? dictionaries.en[key]; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/i18n/index.ts` around lines 5 - 15, TranslationKey is
currently derived from enCommon only, so missing keys in other locale objects
(koCommon) are only caught at runtime; change the dictionaries typing to force
compile-time synchronization by typing dictionaries as Record<Locale,
Record<TranslationKey, string>> (or deriving a Dictionaries type that maps
Locale to Record<keyof typeof enCommon, string>) and adjust createTranslator to
accept Locale and index dictionaries with that type; ensure enCommon and
koCommon conform to that shared Record type so any missing translation keys in
koCommon will cause a TypeScript error during compilation.
| export type AnalysisJobSnapshot = { | ||
| jobId: string; | ||
| request: AnalysisJobRequest; | ||
| status: AnalysisJobStatus; | ||
| startedAt?: string; | ||
| finishedAt?: string; | ||
| error?: AnalysisJobError; | ||
| metadata?: Record<string, unknown>; | ||
| }; |
There was a problem hiding this comment.
AnalysisJobSnapshot.error는 status.error와 쉽게 어긋납니다.
Line 108의 status가 이미 실패 정보를 담는데 Line 111에서 같은 개념을 한 번 더 노출하고 있습니다. 이 상태로는 두 필드가 서로 다른 값을 갖는 snapshot도 타입상 유효해서, 생산자마다 어떤 필드를 진실 원천으로 써야 하는지 모호해집니다.
🔧 수정 예시
export type AnalysisJobSnapshot = {
jobId: string;
request: AnalysisJobRequest;
status: AnalysisJobStatus;
startedAt?: string;
finishedAt?: string;
- error?: AnalysisJobError;
metadata?: Record<string, unknown>;
};별도 의미의 에러가 정말 필요하다면, status.error와 구분되는 이름으로 바꾸고 의미를 문서화하는 편이 안전합니다.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type AnalysisJobSnapshot = { | |
| jobId: string; | |
| request: AnalysisJobRequest; | |
| status: AnalysisJobStatus; | |
| startedAt?: string; | |
| finishedAt?: string; | |
| error?: AnalysisJobError; | |
| metadata?: Record<string, unknown>; | |
| }; | |
| export type AnalysisJobSnapshot = { | |
| jobId: string; | |
| request: AnalysisJobRequest; | |
| status: AnalysisJobStatus; | |
| startedAt?: string; | |
| finishedAt?: string; | |
| metadata?: Record<string, unknown>; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/shared-types/src/index.ts` around lines 105 - 113, The
AnalysisJobSnapshot type currently duplicates failure information by having both
status (which contains error info) and a separate error?: AnalysisJobError
field; remove this ambiguity by deleting the top-level error property from
AnalysisJobSnapshot and rely on status.error as the single source of truth, or
if a distinct error meaning is required, rename error to a clearly
differentiated identifier (e.g., externalError or legacyError) and add a doc
comment explaining how it differs from status.error; update all
references/usages of AnalysisJobSnapshot, AnalysisJobSnapshot.error, and any
code expecting AnalysisJobError to use the chosen single source or new name so
consumers remain consistent.
| function validateAnalysisJobStatus(value: unknown): string | null { | ||
| if (!isRecord(value)) { | ||
| return invalidField("root"); | ||
| } | ||
| const allowedKeysByState: Record<AnalysisJobState, string[]> = { | ||
| queued: ["jobId", "state", "requestedAt", "updatedAt", "progressLabel"], | ||
| running: ["jobId", "state", "requestedAt", "updatedAt", "progressLabel"], | ||
| succeeded: ["jobId", "state", "requestedAt", "updatedAt", "progressLabel", "result"], | ||
| failed: ["jobId", "state", "requestedAt", "updatedAt", "progressLabel", "error"] | ||
| }; | ||
| if (typeof value.jobId !== "string") { | ||
| return invalidField("jobId"); | ||
| } | ||
| if (!isOneOf(ANALYSIS_JOB_STATES, value.state)) { | ||
| return invalidField("state"); | ||
| } | ||
| const extraKey = unexpectedKey(value, allowedKeysByState[value.state], ""); | ||
| if (extraKey) { | ||
| return extraKey; | ||
| } | ||
| if (typeof value.requestedAt !== "string") { | ||
| return invalidField("requestedAt"); | ||
| } | ||
| if (typeof value.updatedAt !== "string") { | ||
| return invalidField("updatedAt"); | ||
| } |
There was a problem hiding this comment.
타임스탬프 필드가 “아무 문자열”이면 통과합니다.
requestedAt와 updatedAt는 계약상 시간값인데 현재는 문자열 여부만 검사합니다. "soon" 같은 값도 유효한 status로 인정돼서, 이후 정렬·표시·polling 기준 계산이 깨질 수 있습니다. createAnalysisJobStatus()가 ISO 문자열을 만들고 있으니 validator도 같은 수준으로 강제하는 편이 맞습니다.
🔧 수정 예시
+function isIsoTimestamp(value: unknown): value is string {
+ return typeof value === "string" && !Number.isNaN(Date.parse(value));
+}
+
function validateAnalysisJobStatus(value: unknown): string | null {
if (!isRecord(value)) {
return invalidField("root");
}
@@
- if (typeof value.requestedAt !== "string") {
+ if (!isIsoTimestamp(value.requestedAt)) {
return invalidField("requestedAt");
}
- if (typeof value.updatedAt !== "string") {
+ if (!isIsoTimestamp(value.updatedAt)) {
return invalidField("updatedAt");
}| if not isinstance(role_focus, list): | ||
| raise ValueError("Invalid analysis job request: invalid field 'roleFocus'") | ||
| for index, role in enumerate(role_focus): | ||
| if not isinstance(role, str): | ||
| raise ValueError(f"Invalid analysis job request: invalid field 'roleFocus[{index}]'") |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if roleFocus usage requires at least one element
rg -n "roleFocus" --type=ts --type=py -C3Repository: seonghobae/bandscope
Length of output: 14211
🏁 Script executed:
#!/bin/bash
# Search for isDenseArray definition
rg -n "isDenseArray" --type=ts -B2 -A5Repository: seonghobae/bandscope
Length of output: 3838
🏁 Script executed:
#!/bin/bash
# Search for roleFocus usage to see if code assumes non-empty
rg -n "roleFocus\[" -A2 -B2 --type=py --type=tsRepository: seonghobae/bandscope
Length of output: 1952
roleFocus 빈 배열 검증 필요
현재 검증 로직에서 roleFocus가 리스트/배열인지와 각 요소가 문자열인지만 확인합니다. 빈 배열 []이 허용되고 있으나, 모든 성공 케이스는 최소 하나 이상의 역할을 포함하고 있습니다. Python과 TypeScript 모두 동일하게 빈 배열을 허용하므로, 빈 배열을 명시적으로 거부하는 검증을 추가하는 것이 좋습니다.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/analysis-engine/src/bandscope_analysis/api.py` around lines 141 -
145, The validation for role_focus currently only checks type and element types
but allows an empty list; update the validation in the same block that uses
role_focus to reject empty lists by adding a check like "if not role_focus:
raise ValueError(...)" so that an empty array raises a clear ValueError for the
'roleFocus' field (keep the existing element-type checks using enumerate for
role strings). Ensure you reference the role_focus variable and the existing
ValueError pattern for 'roleFocus' so the new check is colocated with the
current isinstance and per-element string checks.
| return { | ||
| "jobId": job_id, | ||
| "state": "succeeded", | ||
| "requestedAt": requested_at, | ||
| "updatedAt": requested_at, | ||
| "progressLabel": f"Analysis ready for {request['sourceLabel']}", | ||
| "result": build_demo_rehearsal_song(), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
성공 응답의 result.title이 요청의 sourceLabel과 일치하지 않음
progressLabel은 request['sourceLabel']을 사용하지만, build_demo_rehearsal_song()의 반환값에는 하드코딩된 "Late Night Set" 타이틀이 포함됩니다. 이는 테스트에서 불일치를 야기할 수 있습니다 (test_cli.py Line 53 참조).
♻️ 제안된 수정
-def build_demo_rehearsal_song() -> RehearsalSong:
+def build_demo_rehearsal_song(title: str = "Late Night Set") -> RehearsalSong:
"""Return the bootstrap rehearsal song payload for orchestration tests."""
return {
"id": "demo-song",
- "title": "Late Night Set",
+ "title": title,그리고 run_analysis_job에서:
"progressLabel": f"Analysis ready for {request['sourceLabel']}",
- "result": build_demo_rehearsal_song(),
+ "result": build_demo_rehearsal_song(request['sourceLabel']),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/analysis-engine/src/bandscope_analysis/api.py` around lines 280 -
286, The success response currently mixes a dynamic progressLabel with a
hardcoded demo song title from build_demo_rehearsal_song(), causing test
mismatches; update run_analysis_job to ensure the returned result.title matches
request['sourceLabel']—either by changing build_demo_rehearsal_song to accept a
title parameter and pass request['sourceLabel'], or by overriding
result['title'] = request['sourceLabel'] after calling
build_demo_rehearsal_song(); keep all other fields (jobId, state, timestamps,
progressLabel) unchanged so the contract stays consistent.
| assert song["title"] == "Late Night Set" | ||
| assert song["sections"][0]["roles"][0]["id"] == "bass-guitar" | ||
| assert song["sections"][0]["roles"][2]["manualOverrides"][0]["value"]["source"] == "user" |
There was a problem hiding this comment.
데모 fixture 검증이 실제 계약 깨짐을 못 잡습니다.
지금은 manualOverrides[0].value["source"]만 확인해서 value.chord 누락을 놓칩니다. 현재 demo payload는 value.cord 오타가 있어도 이 테스트는 통과하지만, desktop 쪽에서는 ManualOverridePayload.value를 HarmonyPayload로 역직렬화하므로 실제 실행 시 전체 응답이 invalid로 떨어집니다. 최소한 override value 전체를 단정해서 교차 언어 계약을 여기서 잡아야 합니다.
🔎 제안하는 테스트 보강
- assert song["sections"][0]["roles"][2]["manualOverrides"][0]["value"]["source"] == "user"
+ assert song["sections"][0]["roles"][2]["manualOverrides"][0]["value"] == {
+ "chord": "C#m11",
+ "functionLabel": "vi suspended lift",
+ "source": "user",
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@services/analysis-engine/tests/test_api.py` around lines 64 - 66, The test is
only asserting manualOverrides[0].value["source"], missing validation of the
full override payload which lets typoed keys (e.g., value.cord) pass; update the
test in services/analysis-engine/tests/test_api.py to assert the entire
manualOverrides[0]["value"] matches the expected HarmonyPayload shape (including
the "chord" key and its expected content) or at minimum assert presence and
correctness of required keys like "chord" and "source" on
song["sections"][0]["roles"][2]["manualOverrides"][0]["value"]; reference the
song variable and the manualOverrides/HarmonyPayload contract so the test fails
if the value is malformed or has typos.
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/sbom.yml:
- Around line 41-48: The workflow currently downloads and runs an unaudited Syft
binary in the "Install Syft" / "Generate CycloneDX SBOM" steps; modify the job
to (1) pin or bump the Syft version to a current stable release (replace v1.20.0
with the chosen version string used in the workflow), (2) download the release
artifacts and verify the SHA-256 checksum before extracting/executing (compute
and compare against a hardcoded checksum value you add to the workflow for the
syft_*.tar.gz file), and (3) add an entry for Syft into
supply-chain/supplemental-component-inventory.json including source URL,
version, SHA-256 checksum, and license fields to satisfy
docs/security/dependency-policy.md requirements; ensure the workflow errors out
if checksum verification fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3dc4e09e-ce08-477b-8cfc-19ec31b38498
📒 Files selected for processing (1)
.github/workflows/sbom.yml
| - name: Install Syft | ||
| run: | | ||
| curl -sSfL -o "$RUNNER_TEMP/syft.tar.gz" "https://github.com/anchore/syft/releases/download/v1.20.0/syft_1.20.0_linux_amd64.tar.gz" | ||
| tar -xzf "$RUNNER_TEMP/syft.tar.gz" -C "$RUNNER_TEMP" syft | ||
|
|
||
| - name: Generate CycloneDX SBOM | ||
| uses: anchore/sbom-action@57aae528053a48a3f6235f2d9461b05fbcb7366d # v0.23.1 | ||
| with: | ||
| path: . | ||
| format: cyclonedx-json | ||
| output-file: bandscope-sbom.cdx.json | ||
| run: | | ||
| "$RUNNER_TEMP/syft" dir:. -o cyclonedx-json=bandscope-sbom.cdx.json |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
anchore syft latest release version 2026
💡 Result:
Anchore Syft latest release (as of March 12, 2026) is v1.42.2, published March 9, 2026. [1]
Source: GitHub releases page for anchore/syft. [1]
🏁 Script executed:
# Check if dependency policy exists
find . -name "dependency-policy.md" -o -name "*security*" -type f | head -20Repository: seonghobae/bandscope
Length of output: 281
🏁 Script executed:
cat -n ./docs/security/dependency-policy.mdRepository: seonghobae/bandscope
Length of output: 6804
🏁 Script executed:
# Check if supplemental inventory exists and contains Syft
find . -name "supplemental-component-inventory.json" -type fRepository: seonghobae/bandscope
Length of output: 116
🏁 Script executed:
cat -n ./supply-chain/supplemental-component-inventory.jsonRepository: seonghobae/bandscope
Length of output: 448
🏁 Script executed:
# Check the .github/workflows/sbom.yml file to understand context
head -50 .github/workflows/sbom.ymlRepository: seonghobae/bandscope
Length of output: 1229
🏁 Script executed:
cat -n scripts/checks/verify_supply_chain.py | head -100Repository: seonghobae/bandscope
Length of output: 4825
Syft 바이너리 다운로드가 의존성 정책 요구사항을 위반합니다.
이 변경사항은 저장소의 의존성 정책(docs/security/dependency-policy.md)의 두 가지 중요한 요구사항을 위반합니다:
-
체크섬 검증 누락 (정책 72줄): 릴리스 산출물은 체크섬이나 동등한 무결성 데이터를 포함해야 합니다. 현재 코드는 외부 바이너리를 검증 없이 다운로드하고 실행하므로 공급망 공격에 취약합니다.
-
추적 누락 (정책 70줄): Syft는
supply-chain/supplemental-component-inventory.json에 추적되어야 하는 번들된 바이너리입니다. 현재 인벤토리는 비어있습니다.
또한 Syft v1.20.0은 현재 최신 릴리스인 v1.42.2(2026년 3월 9일)에서 22개 마이너 버전 뒤처져 있어 보안 패치가 누락되어 있습니다.
다음을 수정해야 합니다:
- SHA-256 체크섬 검증 추가
- Syft를 supplemental-component-inventory.json에 추가 (소스 URL, 버전, 체크섬, 라이선스 포함)
- 최신 안정 버전으로 업그레이드 고려
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/sbom.yml around lines 41 - 48, The workflow currently
downloads and runs an unaudited Syft binary in the "Install Syft" / "Generate
CycloneDX SBOM" steps; modify the job to (1) pin or bump the Syft version to a
current stable release (replace v1.20.0 with the chosen version string used in
the workflow), (2) download the release artifacts and verify the SHA-256
checksum before extracting/executing (compute and compare against a hardcoded
checksum value you add to the workflow for the syft_*.tar.gz file), and (3) add
an entry for Syft into supply-chain/supplemental-component-inventory.json
including source URL, version, SHA-256 checksum, and license fields to satisfy
docs/security/dependency-policy.md requirements; ensure the workflow errors out
if checksum verification fails.
|
Superseded by #47 to clear repeated stale CodeRabbit review state after the latest orchestration hardening fixes and green verification. |
Pull request was closed
Summary
Verification
./scripts/harness/quickcheck.shcargo check --manifest-path apps/desktop/src-tauri/Cargo.toml